OLS-3593 enable init container comparison for app server deployment - #1897
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/retest |
56a1912 to
7eac125
Compare
openshift-ci
Bot
added
the
needs-rebase
The app server deployment update check called DeploymentSpecEqual with compareInitContainers=false, so changes to RAG images in .spec.ols.rag were never detected. Simply enabling init container comparison causes an infinite reconciliation loop because OpenShift's image trigger controller resolves ImageStreamTags to SHA digests, which the operator then overwrites with the original tag on the next reconcile. Instead, track a SHA-256 hash of the RAG spec in a deployment annotation. When the user changes .spec.ols.rag, the hash changes and triggers a deployment rollout without conflicting with the image trigger mechanism. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
7eac125 to
621853e
Compare
openshift-ci
Bot
removed
the
needs-rebase
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: raptorsun The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
blublinsky
left a comment
blublinsky
left a comment
There was a problem hiding this comment.
Review: OLS-3593 RAG spec hash detection
Nice, clean fix for a subtle bug. The hash-based approach avoids the image trigger controller fight elegantly.
Non-blocking observations
-
json.Marshalerror discarded (deployment.go:ragSpecHash()) —data, _ := json.Marshal(...)drops the error. In practiceRAGSpecis a simple struct and won't fail, but a//nolint:errcheckcomment or a brief rationale would help future readers. Not blocking sincesha256.Sum256(nil)is deterministic (no spurious rollouts). -
No unit test for the new logic — a table-driven test for
ragSpecHash()(empty / single / multiple entries) and one case inupdateOLSDeploymentverifying the annotation-based rollout would be straightforward additions. Not blocking since e2e coverage exists.
Both are nice-to-haves. LGTM otherwise — the approach is sound and the PR description is excellent.
|
/lgtm |
|
/test bundle-e2e-4-21 |
|
/test bundle-e2e-4-21 |
|
@raptorsun: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
.spec.ols.rag(RAG image references) so they trigger a deployment rolloutDeploymentSpecEqualwas called withcompareInitContainers=false, and no other change detector tracked RAG images — so RAG image changes were silently ignoredWhy not just enable init container comparison?
The obvious fix — flipping
compareInitContainersfromfalsetotrue— causes an infinite reconciliation loop when RAG is configured.When RAG images are present, the operator sets an
image.openshift.io/triggersannotation on the deployment (rag.go:62-79). This tells OpenShift's image trigger controller to resolve ImageStreamTags and replace the init container image field with the resolved@sha256:...digest. WithcompareInitContainers=true, this creates a fight between two controllers:my-image:latest)my-image@sha256:abc...)ContainerSpecEqualcomparesa.Image == b.Image, sees tag != digest, detects a "change"This was confirmed in e2e testing where the deployment generation reached 5663 vs observed generation 2303 (~5-6 updates/second).
Solution
Track a SHA-256 hash of the CR's
.spec.ols.ragin a deployment annotation (ols.openshift.io/rag-spec-hash). When the user changes RAG image references, the hash changes and triggers a deployment rollout — without comparing init container images directly and without conflicting with OpenShift's image trigger mechanism.Test plan
make testpasses — all unit tests greenbundle-e2e-4-21CI passed (including BYOK/RAG tests)Fixes: OLS-3593
🤖 Generated with Claude Code